-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[fix] fix all cve warnings #138
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CR: I'm a bit sceptical against installing "D".
FYI: Since we are pushing a docker image on build (which is scanned by Grafana using trivy
). I ran trivy image har-to-k6:local
, which resulted in Total: 57 (UNKNOWN: 0, LOW: 50, MEDIUM: 5, HIGH: 1, CRITICAL: 1)
Not asking you to fix the image vulnerabilities, just pointing out that they are subjected to show up when grafana scans the docker image.
package.json
Outdated
@@ -29,6 +29,7 @@ | |||
"bundle-collapser": "^1.3.0", | |||
"caporal": "1.0.0", | |||
"chalk": "^2.4.2", | |||
"D": "^1.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
This package has been deprecated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A guess: npm install D
😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the Dockerfile to use node:20-alpine
to minimize the CVEs to 2 medium ones. It also shrinks the image by 60 MB.
D
has been removed.
I also noticed a warning that babel-eslint
was no longer updated, so I switched to @babel/eslint-parser
to make any future updates easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👌🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with success and no warnings 👍
This PR fixes all the CVE warnings when running ´trivy
and
npm audit` on the repo.The critical CVE was easy to fix. We just needed to upgrade babel to the latest version and it went away.
The other warnings were caused by an old version of ava, and that proved to be trickier to fix. The old version had support for mixing ESM and CJS, but that was dropped in favor of native esm support in node. But that has some problems:
type
property in package.json: iftype
is"module"
CJS modules need to have the extension.cjs
and vice versa.In the end, the simplest solution was to just convert all the
import
statements in the test suite torequire
statements.How to test
trivy fs . --include-dev-deps
in the repositorynpm audit
in the reposityExpected result
There should be no CVEs.